-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display warning on preview environment to notify users if page is Mozart #12038
base: latest
Are you sure you want to change the base?
Conversation
…en code deployed to test environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
const [isPreview, setIsPreview] = useState(false); | ||
|
||
useEffect(() => { | ||
const hostnameMatchesPreview = window.location.hostname.match(/preview/g); | ||
const isPreviewEnvironment = | ||
hostnameMatchesPreview && hostnameMatchesPreview.length > 0; | ||
|
||
setIsPreview(isPreviewEnvironment); | ||
}, []); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to double check that no CLS or LCP issues happen or any weird behaviour with child components, as using state might cause a re-render.
Not actually sure if react does a re-render when you set an already false state value to false, hopefully it's clever enough, or you could be doubly safe and just set the state when isPreviewEnvironment is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosh I never thought about LCP/CLS 😬 Luckily this is only on the preview environment, but all the same, it's best to check. I'll ensure not to set state unless preview env is true & see if that causes a re-render or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But of course this code will run regardless of which environment it's on. I'll do a quick comparison between the test environment & preview with this banner to see if there's a difference in Web Vital scores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine, as I don't think there's anything that will actually make react change anything in the DOM, but best to double check
Resolves JIRA N/A
Overall changes
Displays a warning on the preview environment if a page is rendered by Mozart (meaning that it cannot pick up changes from branch/PR)
Code changes
Add an indicator component to the header/footer
Add
requestServiceChain
to the request contextTesting
Locally - cannot test on preview for pages not routed directy to Simorgh as they are rendering the test.bbc.com version
Unit tests
Preview environment: indicator not displayed for pages routed directly to Simorgh
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines